Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The init command does not return SSL errors #4479

Merged
merged 4 commits into from
Nov 4, 2021
Merged

The init command does not return SSL errors #4479

merged 4 commits into from
Nov 4, 2021

Conversation

fguimond
Copy link
Contributor

@fguimond fguimond commented Nov 4, 2021

The SSL or other connection error is now logged as part of the context error.

Signed-off-by: Francis Guimond [email protected]

What is this change?

On backend init the SSL error generated when connecting to etcd was not logged at all, making troubleshooting complex. Change the behaviour to log the SSL error (or any other connectivity error) alongside the context error.

Closes #3663

Why is this change necessary?

To help with troubleshooting etcd connection issues.

Does your change need a Changelog entry?

Created one.

Do you need clarification on anything?

No.

Were there any complications while making this change?

No.

Have you reviewed and updated the documentation for this change? Is new documentation required?

No.

How did you verify this change?

Manual testing.

docker volume create --name etcd-data
export NODE1=192.168.7.160
export DATA_DIR="etcd-data"
export REGISTRY=gcr.io/etcd-development/etcd
docker run
   -p 2379:2379 \
   -p 2380:2380 \
   --volume=${DATA_DIR}:/etcd-data \
   --volume=/etc/sensu/tls/:/certs \
   --name etcd ${REGISTRY}:latest \
   /usr/local/bin/etcd \
   --data-dir=/etcd-data \
   --name node1 \
   --initial-advertise-peer-urls https://${NODE1}:2380 \
   --listen-peer-urls https://0.0.0.0:2380 \
   --advertise-client-urls https://${NODE1}:2379 \
   --listen-client-urls https://0.0.0.0:2379 \
   --initial-cluster node1=https://${NODE1}:2380 \
   --cert-file /certs/etcd1.pem \
   --key-file /certs/etcd1-key.pem \
   --peer-cert-file /certs/etcd1.pem \
   --peer-key-file /certs/etcd1-key.pem \
   --trusted-ca-file /certs/ca.pem \
   --peer-trusted-ca-file /certs/ca.pem
  • Edit your /etc/hosts file to add the following line
192.168.7.160 etcd1 dummyhost
  • Create the following backend.yml file
state-dir: "./backend-secure-etcd/state"
cache-dir: "./backend-secure-etcd/cache"
log-level: "info"
no-embed-etcd: true
etcd-cert-file: "./tls/agent.pem"
etcd-key-file: "./tls/agent-key.pem"
etcd-trusted-ca-file: "./tls/ca.pem"
etcd-client-urls: "https://dummyhost:2379"
timeout: "15s"
  • Launch backend init using that file:
sensu-backend init -c ./backend-secure-etcd/backend.yml --cluster-admin-username admin --cluster-admin-password P@ssw0rd!

The following error should be logged.

{"component":"cmd","level":"error","msg":"error connecting to etcd endpoint: context deadline exceeded: connection error: desc = \"transport: authentication handshake failed: x509: certificate is valid for localhost, etcd1, not dummyhost\"","time":"2021-11-04T11:16:41-04:00"}

Is this change a patch?

No.

@fguimond
The init command does not return SSL errors
8317b08 
The SSL or other connection error is now logged as part of the context error.

Signed-off-by: Francis Guimond <[email protected]>
@fguimond
The init command does not return SSL errors
cda7ba7 
CHANGELOG.md entry

Signed-off-by: Francis Guimond <[email protected]>
@fguimond fguimond requested a review from ccressent November 4, 2021 18:17
ccressent
ccressent reviewed Nov 4, 2021
View reviewed changes
Copy link
Contributor

@ccressent ccressent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I can't believe it was that simple in the end! :)

I a have slight concern though: the documentation says that it's "experimental". Should we also keep the WithBlock() option explicitly, in case WithReturnConnectionError() stops implying it for some reason?

@fguimond
The init command does not return SSL errors
681da00 
Explicitly specify WithBlock dial option for etcd

Signed-off-by: Francis Guimond <[email protected]>
@fguimond
Copy link
Contributor Author

fguimond commented Nov 4, 2021

Looks good and I can't believe it was that simple in the end! :)

I a have slight concern though: the documentation says that it's "experimental". Should we also keep the WithBlock() option explicitly, in case WithReturnConnectionError() stops implying it for some reason?

Good point @ccressent, thanks for catching that!

Fixed.

@fguimond fguimond merged commit 7902137 into main Nov 4, 2021
@fguimond fguimond deleted the backend-init-ssl-error branch November 4, 2021 19:48
@calebhailey
Copy link

This is fantastic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccressent ccressent ccressent approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The init command does not return SSL errors
3 participants
@fguimond @calebhailey @ccressent